-
Notifications
You must be signed in to change notification settings - Fork 558
Fix structured output backwards compatibility for string results #748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix structured output backwards compatibility for string results #748
Conversation
| // If there is structuredOutput we must stringify it, as the MCP specification requires Content to be for backwards compatibility | ||
| // but otherwise not differ | ||
| Content = [new TextContentBlock { Text = structuredContent == null ? text : | ||
| JsonSerializer.Serialize(structuredContent, AIFunction.JsonSerializerOptions.GetTypeInfo(typeof(object)))}], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just resolve the type of structuredContent itself and skip one degree of indirection?
| JsonSerializer.Serialize(structuredContent, AIFunction.JsonSerializerOptions.GetTypeInfo(typeof(object)))}], | |
| JsonSerializer.Serialize(structuredContent, AIFunction.JsonSerializerOptions.GetTypeInfo<JsonNode>()))}], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just resolve the type of structuredContent itself and skip one degree of indirection?
Suggested change
JsonSerializer.Serialize(structuredContent, AIFunction.JsonSerializerOptions.GetTypeInfo(typeof(object)))}], JsonSerializer.Serialize(structuredContent, AIFunction.JsonSerializerOptions.GetTypeInfo<JsonNode>()))}],
Good point, but this goes for the default switch case too then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I suppose we can leave it for consistency.
src/ModelContextProtocol.Core/Server/AIFunctionMcpServerTool.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Eirik Tsarpalis <[email protected]>
|
Should an error be thrown on the AIContent related return types? If we go by the intent expressed by dsp and others, and It would be a breaking change, so probably best to wait with that until the spec is actually updated. Just wanted to mention that as pointed out in the issue, this is only a fix for the string case. I think that's the one most likely to cause trouble on actual production servers, so I still think it's worth fixing this one in isolation. Setting the attribute property to true for an AIContent or CallToolResult returning tool is a really exotic edge-case (to be fixed with the spec change imo - at the same time as any introduced schema validation requirements). |
|
For consideration: https://developers.openai.com/apps-sdk/build/mcp-server a sample response from their "Structure the data your tool returns" paragraph: |
There is some discussion around this at the protocol governance level. I expect we'll see a change where they do not have to be the same. The current requirements of the Apps SDK will cause some MCP SDKs to warn/fail, and isn't fully spec compliant, even if one can argue the spec doesn't forbid the discrepancy, this is because the assumption was they either matched or only one was provided. Another interesting aspect is that the Apps SDK sends the unstructured text result to the model and uses the structured results for host specific processing. Implementing something similar on the client host in the C# SDK is awkward, as the convenience MEAI tool mappers assume the structured output should be sent to the model. So the change in the spec should arguably also be accompanied by a change to this behavior to prioritize the unstructured output if supplied. Or at least make it easily configurable. |
From what i understand from the OpenAI Apps SDK docs is that both the structured AND the unstructured content are provided to the model: |
Ah yes - but I am not sure all of that is placed in the actual tool response content part. Either way, we're now in a situation where it is client dependent which fields go where and how, which complicated things a lot when one wants to write servers that are generic and work with multiple clients. |
Fixes #747
We might want to warn or give errors if
StructuredContentis present when there are incompatible content blocks - ie more than one text content block. I don't think it can actually happen, but if it can it will be a spec violation. Clients should not get different output in unstructuredContentifStructuredContentis provided.